Race condition check: new volunteers system spec#6745
Race condition check: new volunteers system spec#6745compwron merged 6 commits intorubyforgood:mainfrom
Conversation
It avoids hitting the Database when running system tests
Keep volunteer tests in the same context, and have each test creating its own required setup.
The system test was not checking for anything on the page. This test is better placed at the model level, since it's related to business logic.
Since we're not checking for anything related to an organization, we can speed this test by building factories instead of creating them.
stefannibrasil
requested review from
FireLemons,
compwron and
elasticspoon
as code owners
|
Nice nice nice :) |
There was a problem hiding this comment.
Pull request overview
Updates volunteer-related specs to reduce race-condition risk in system tests by preferring assertions that reflect what a user sees in the browser, and relocates invitation-email expectations to a model spec.
Changes:
- Refactors
spec/system/volunteers/new_spec.rbto avoid backend-only assertions and instead verify UI-visible outcomes. - Moves the invitation email assertions from the system spec into
spec/models/volunteer_spec.rb. - Reorganizes volunteer-related contexts and tweaks factory usage to reduce unnecessary setup overhead.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| spec/system/volunteers/new_spec.rb | Removes admin invitation expectations from the system spec; updates supervisor flow to assert via UI-visible content. |
| spec/models/volunteer_spec.rb | Adds a model-level spec asserting invitation email delivery and invitation timestamp behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| volunteer = build(:volunteer, email: "new_volunteer@example.com") | ||
| volunteer.invite! | ||
|
|
||
| email = ActionMailer::Base.deliveries.last | ||
|
|
||
| expect(email).not_to be_nil | ||
| expect(email.to).to eq ["new_volunteer@example.com"] | ||
| expect(email.subject).to eq("CASA Console invitation instructions") |
There was a problem hiding this comment.
This example only checks ActionMailer::Base.deliveries.last, which can be a weaker assertion if other mail is delivered in the same example (or if delivery behavior changes). Consider asserting the delivery count change around volunteer.invite! (and then using the resulting last email) so the spec proves the invite action triggered exactly one email.
What github issue is this PR for, if any?
Related #6698
What changed, and why?
How is this tested? (please write rspec and jest tests!) 💖💪
Everything is a test ⭐